Add factory:setup + upfront prerequisite checks for pnpm factory:go - #5591
Conversation
Host Test Results 1 files ± 0 1 suites ±0 2h 33m 21s ⏱️ + 8m 37s Results for commit 38ecdac. ± Comparison against earlier commit 978060f. Realm Server Test Results 1 files ± 0 1 suites ±0 11m 22s ⏱️ - 1m 34s Results for commit 38ecdac. ± Comparison against earlier commit 978060f. For more details on these errors, see this check. |
A fresh checkout could not run pnpm factory:go without four undocumented manual fixes, each surfacing as a separate opaque crash from deep inside a run: no working Node pin, an unbuilt boxel-cli dist/api.js, a host build lacking dev-mode test entries, and a missing Playwright browser binary. - Add src/preflight.ts: detects all four prerequisites (Node >= 24, boxel-cli/dist/api.js, host/dist/tests/index.html, Playwright chromium-headless-shell) without importing boxel-cli, so the check runs before the dist/api.js import that would otherwise crash first. - factory-entrypoint.ts now runs preflight up front and defers the boxel-cli import until it passes, reporting every missing prerequisite at once and pointing at pnpm factory:setup. --help still works. - Add scripts/factory-setup.ts (pnpm factory:setup): idempotent bootstrap that builds the missing artifacts and installs the Playwright browser, skipping anything already present (--force rebuilds all). - Add a real volta pin to the root package.json so the existing volta.extends chain in software-factory resolves a Node version. - Document the one-time setup and Node requirement in the README. CS-12186 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
b5a9fc1 to
497c830
Compare
Two integration cases move HOME to isolate the boxel profile, but preflight resolves the Playwright browser cache from HOME as well. Under a temp home it finds no `chromium_headless_shell-*` build and refuses to run the factory, so the `--debug` summary case exits 1 instead of 0 and the no-profile case reports a missing browser binary rather than the `boxel profile add` guidance it asserts. Installing browsers in CI doesn't help — they are installed, just not under the temp home. Pin those two children to the cache the test process itself resolves, so only the profile is isolated. `playwrightBrowsersRoot` is exported for that; it returns undefined only under `PLAYWRIGHT_BROWSERS_PATH=0`, which the inherited env already carries through. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`factory-target-realm.spec.ts` relocates HOME for the same profile-isolation reason as the integration cases, so preflight aborts there as well and no target realm is ever created. The spec asserts `status === 1` because the loop fails without an API key, and a preflight abort exits 1 too — so that assertion still passed and the run failed later, as a 404 fetching a session for the realm that was never made. Move the browser-cache env into `tests/helpers/preflight-env.ts` and use it at all three spawn sites rather than repeating it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…stackboxel-checkout-cannot-run-pnpm-factorygo
habdelra
left a comment
habdelra
left a comment
There was a problem hiding this comment.
[Claude Code 🤖]
Review: factory:setup + upfront preflight for pnpm factory:go
I focused this review on the failure-ordering contract that makes the fix work — preflight must run before the @cardstack/boxel-cli/api import, and both preflight and factory:setup must load without that build artifact present — plus whether the detection heuristics and version pins actually match the code they claim to. Verified empirically against the checked-out head branch where checkable.
Bottom line: no blocking issues. The design attacks the right root cause (upfront validation + a deferred boxel-cli import) rather than committing build artifacts, and the load-bearing invariants hold.
What lands right (verified)
- Deferred-import ordering.
missingPrerequisites()runs beforeimport('@cardstack/boxel-cli/api'), and every static top-level import insrc/cli/factory-entrypoint.ts(setup-logger,logger,preflight) is boxel-cli-free — so a fresh checkout gets the consolidated message instead ofERR_MODULE_NOT_FOUND. Confirmedpreflight.tsloads with nodist/api.jspresent and returns a correct satisfied/missing set. - Version pins line up.
.nvmrc=24.17.0, rootdevEngines.runtime=24.17.0, and the new rootvolta.node=24.17.0;packages/software-factorycarriesvolta.extends: "../../package.json", so the previously dead-ended extends chain now resolves.PINNED_NODE/MIN_NODE_MAJORmatch. - Build claims are accurate.
@cardstack/host'sbuildisvite build --mode=development, and the vite config comment explicitly names "software-factory tests" as a dev-mode-build consumer — sodist/tests/index.htmlis emitted by exactly the command thefixpoints at.boxel-cli'sbuild:apiscript exists and its./apiexport resolves to the gitignored./dist/api.js. - Test env fix is necessary and correct. Because preflight resolves the Playwright cache from
HOME, the relocated-HOMEtests would fail preflight before reaching the profile/arg errors they assert; spreadingplaywrightBrowsersEnvafter theHOMEoverride pins the child to the real cache while still isolating the profile. The helper's doc comment matches its behavior.
Non-blocking recommendations
wantsHelpin the CLI duplicateswantsFactoryEntrypointHelpin the module — forced by the deferred import, but a drift risk. Suggest a single home inpreflight.ts. (see inline thread onwantsHelp)- Guard the boxel-cli-free invariant for the next editor — it also binds
factory-setup.ts→setup-logger.ts→logger.ts. (see inline thread on thepreflight.tsheader) - Test hermeticity. With preflight now running inside the spawned
factory:go, the two tests that assert arg-parse / usage behavior ("fails clearly when required inputs are missing", "prints usage with --help") can only reach those assertions once every prerequisite is provisioned. In particular, "fails clearly when required inputs are missing" carries neither a relocatedHOMEnorplaywrightBrowsersEnv, so on a machine that has never runfactory:setup/playwright installit will fail with the preflight "Cannot run the factory" message instead of its assertedMissing required --brief-url. That's an acceptable consequence of testing the real entrypoint — flagging only that these two are no longer hermetic; no change requested. - Evergreen comments (low priority). The new comments carry a tracker ID (the
CS-12186reference in thepreflight.ts/factory-setup.ts/ CLI headers). The repo's evergreen-comments guidance discourages tracker refs in code comments, and these read fine without the ID, which will outlive the ticket. The surrounding code already carries legacyCS-…refs, so this is convention-consistency, not a regression.
Confirmations on the scope notes
- The out-of-scope stale
dist/api.jscase checks out:import('@cardstack/boxel-cli/api')resolves for a stale-but-present bundle, andlet { BoxelCLIClient } = clisimply bindsundefined, deferring the failure toBoxelCLIClient.ensureProfile(caught, logged as a stack).--forceis the escape hatch. Reasonable to defer. - One nuance on the description: "
--helpstill works" holds on a provisioned checkout. On a checkout that hasn't builtdist/api.js,--helpnow prints the prerequisites list and exits 1 (preflight is skipped for help, the deferred import throws, and thecatchre-runs the check). The in-code comment is precise about this, and the behavior is arguably more useful than raw usage — I'd leave it; just noting the description slightly overstates it.
Generated by Claude Code
| function wantsHelp(argv: string[]): boolean { | ||
| let normalized = argv[0] === '--' ? argv.slice(1) : argv; | ||
| return normalized.includes('--help'); | ||
| } |
There was a problem hiding this comment.
[Claude Code 🤖]
Non-blocking — drift-by-duplication. This wantsHelp is identical in logic to wantsFactoryEntrypointHelp in the module (src/factory-entrypoint.ts):
let normalized = argv[0] === '--' ? argv.slice(1) : argv;
return normalized.includes('--help');The duplication is forced — you can't call the module's version before the deferred @cardstack/boxel-cli/api import, and running the help check up front is the whole point. But two copies of the same argv rule drift the first time one learns something (adding a -h short flag, accepting bare help, etc.): the CLI would skip preflight while the module still renders usage, or vice-versa, and nothing fails loudly — the two decisions just quietly disagree.
Single home: put this predicate in preflight.ts (it imports nothing from boxel-cli and is already statically imported here), then import it in both the CLI and the module; wantsFactoryEntrypointHelp can become a thin re-export for its existing callers.
Not blocking — the copies are correct today.
Generated by Claude Code
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 38ecdac — and gone rather than shared, which turned out to be possible.
Your read of the constraint was exactly right: the CLI has to answer the help question before importing the entrypoint module, so it could not call the module's copy. But it did not need a second copy either. The predicate now lives in preflight.ts — already statically imported here, boxel-cli-free by construction, and it imports nothing back, so there is no cycle — and the CLI uses it directly:
let helpRequested = wantsFactoryEntrypointHelp(argv);src/factory-entrypoint.ts re-exports it (export { wantsFactoryEntrypointHelp } from './preflight.ts'), so the four assertions in factory-entrypoint.test.ts and any other importer keep reaching it under the same name with no change.
One extra bit of duplication fell out with it: the CLI was calling the predicate a second time after the deferred import (if (wantsFactoryEntrypointHelp(argv))) having already computed helpRequested at the top. That branch now reuses the value, so the argv rule is consulted once per run and defined once in the tree — adding -h later is a one-line change with no way for the two sides to disagree.
| // It deliberately imports nothing from `@cardstack/boxel-cli` — one of the | ||
| // prerequisites it checks for is boxel-cli's own `dist/api.js`, so importing it | ||
| // here would defeat the purpose (the import would crash before the check runs). |
There was a problem hiding this comment.
[Claude Code 🤖]
Confirmation — this is the load-bearing invariant of the PR, worth guarding explicitly. The comment is exactly right, and I verified it: importing ./preflight.ts in a worktree with no packages/boxel-cli/dist/ present loads cleanly and returns a sane prerequisite set. So pnpm factory:setup — the thing that builds dist/api.js — can itself run on a truly fresh checkout without the ERR_MODULE_NOT_FOUND it exists to cure.
One thing for the next editor, because it's one indirection away and easy to miss: the same invariant binds factory-setup.ts's transitive imports. It pulls in setup-logger.ts → logger.ts, which only require('@cardstack/logger') (a published CJS package, no build step) — not boxel-cli. If anything in the preflight.ts / setup-logger.ts / logger.ts chain ever grows a runtime import of @cardstack/boxel-cli/api, factory:setup regresses to crashing on the exact fresh checkout it's meant to fix, and the failure would look unrelated to the change that caused it. A one-line reminder here (or on the factory-setup.ts import block) would make that trap visible.
Nothing to change.
Generated by Claude Code
There was a problem hiding this comment.
[Claude Code 🤖] Taken — the reminder is in, in 38ecdac. You were right that the trap is one indirection away and that the failure would not point at its cause, which is exactly the kind of thing worth writing down rather than trusting the next reader to re-derive.
The header now names the chain explicitly:
That constraint is load-bearing for more than this file:
factory:setupis what buildsdist/api.js, and it reaches this module throughscripts/factory-setup.ts→setup-logger.ts→logger.ts. Every link in that chain has to stay boxel-cli-free. If one of them grows a runtime import of@cardstack/boxel-cli/api,factory:setupstarts crashing on exactly the fresh checkout it exists to fix, and the failure won't look related to whatever change caused it.
I re-verified the chain while writing it: logger.ts only requires @cardstack/logger (published CJS, no build step), setup-logger.ts imports just logger.ts, and this module imports nothing but node builtins. So the invariant holds today as you found it.
The same commit also dropped the tracker ID from your fourth summary point — it was in three places (this header, the CLI preflight comment, and factory-setup.ts) and, as you noted, all three read fine without it.
The argv rule for `--help` existed twice: the CLI needs the answer before it can import the entrypoint module, since that import pulls in boxel-cli. Both copies were correct, but they'd have disagreed silently the first time one of them learned a new spelling. It now lives in preflight.ts — which the CLI already imports and which is boxel-cli-free by construction — and the entrypoint module re-exports it so its existing callers are unaffected. The CLI reuses the value it already computed rather than asking a second time. Record that the boxel-cli-free constraint covers more than preflight.ts: it also binds factory-setup.ts and the setup-logger/logger chain it pulls in. Breaking any link there makes factory:setup crash on the fresh checkout it exists to fix, with a cause that looks unrelated. Skip the two tests that assert on argument handling when prerequisites are missing, but never in CI. They spawn the real package script, so preflight now refuses inside the child before arg parsing runs — meaning they failed on exactly the fresh checkout this work is about, pointing at arg parsing when the cause was unbuilt artifacts. In CI the artifacts are built as part of the run, so a missing prerequisite stays a loud failure. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
[Claude Code 🤖] All five points addressed in 38ecdac (plus a description fix). The two with inline threads are answered there; these are the other three. Test hermeticity (your #3) — acted on, despite the waiver. You flagged this as an acceptable consequence with no change requested. I went ahead anyway, because of where the failure lands: both tests would fail on precisely the fresh checkout this PR exists to improve, and they would fail pointing at argument parsing when the real cause is unbuilt artifacts. Someone who clones, installs, and runs the suite gets the most misleading possible version of the problem preflight was added to explain. Confirmed both halves of your analysis first:
Both are now defined as (First attempt used an early Evergreen comments (your #4) — done. Three occurrences removed ( The Verification. Also worth noting for anyone reading the earlier red CI on this PR: the host-test failure was the |
2 participants
Problem
On a fresh
pnpm install-only checkout,pnpm factory:gocould not run end-to-end. It failed with a chain of four distinct blockers, each surfacing as a separate opaque crash from deep inside a run, and each requiring monorepo-internals knowledge to fix:package.jsonrequiresnode >=24, but the only pin was.nvmrc(Volta doesn't read it);software-factory'svolta.extends: "../../package.json"dead-ended because the root had novoltablock.boxel-cli/dist/api.js— the entrypoint imports@cardstack/boxel-cli/api, which resolves to a gitignoreddist/api.jsthat nothing in thefactory:gopath builds →ERR_MODULE_NOT_FOUNDat module load.boxel test's harness needshost/dist/tests/index.html, only emitted by a dev-mode host build.boxel testlaunches headless Chromium; the binary was never downloaded.None were documented, and no error pointed at the next blocker in the chain.
Fix
Attacks the root cause (
factory:godid zero upfront validation) rather than committing build artifacts:src/preflight.ts(new) — detects all four prerequisites. Imports nothing from boxel-cli, so it can run before thedist/api.jsimport that used to crash first.src/cli/factory-entrypoint.ts— runs preflight up front and defers the boxel-cli import until it passes. Missing prerequisites produce one consolidated message pointing atpnpm factory:setup; every missing item is listed with its individual fix command. Preflight is skipped for--help, so usage stays available on a provisioned checkout; on one that hasn't builtdist/api.jsthe deferred import throws and the catch re-runs the check, so--helpprints the prerequisites list and exits 1 — more useful there than raw usage.scripts/factory-setup.ts+factory:setupscript (new) — idempotent bootstrap that builds the missing artifacts and installs the Playwright browser, skipping anything already present.--forcerebuilds everything.package.json— added a realvoltapin (node: 24.17.0, matching.nvmrc/devEngines) so the existingvolta.extendschain resolves.Verification
--helpon a provisioned checkout → usage, exit 0; on an unprovisioned one → the prerequisites list, exit 1.dist/api.js→ clean consolidated message + exit 1 (noERR_MODULE_NOT_FOUND).factory:setupno-op run skips all three steps, exit 0.ember-tsc --noEmitclean.Out of scope
This handles missing artifacts on a fresh checkout. A stale
dist/api.js(present but built from older source) still passes the existence check yet can crash on a missing named export;factory:setup --forcefixes it, but automatic staleness detection is a possible follow-up.🤖 Generated with Claude Code